Skip to content

Remove unnecessary Policies's method & add new util functions#63

Merged
arlolra merged 2 commits into
masterfrom
policy
Aug 12, 2016
Merged

Remove unnecessary Policies's method & add new util functions#63
arlolra merged 2 commits into
masterfrom
policy

Conversation

@vqhuy
Copy link
Copy Markdown
Member

@vqhuy vqhuy commented Aug 12, 2016

This pull is a part of #29. It includes following changes:

  • Remove unnecessary method from merkletree/policy.go.
  • Add new functions to util package.
  • sign.GenerateKey should take a reader as vrf.

@vqhuy
Copy link
Copy Markdown
Member Author

vqhuy commented Aug 12, 2016

Btw, I'm a little bit uncomfortable when looking at the sign.go and vrf.go right now. They both use ed25519 but they have difference GenerateKey's signatures, and difference key type (sign's key type is a slice and vrf's key type is an array). Just a few questions came to my mind:

  • Do we need to return the public part in GenerateKey?
  • What should be returned by key.Public()? (a PublicKey or a slice of byte?)
  • What should be the underlying type of the key: a slice or an array? I'd prefer a slice.

I suggest we should refactor the vrf package to export the same API's signature & key type as sign package. What do you think?
@arlolra @liamsi @masomel

@liamsi
Copy link
Copy Markdown
Member

liamsi commented Aug 12, 2016

Do we need to return the public part in GenerateKey?

If we don't, we assume the key to be of a specific format (like the one created by ed25519). Not returning the public-key here could make #50 more difficult to implement.

What should be returned by key.Public()? (a PublicKey or a slice of byte?)
What should be the underlying type of the key: a slice or an array? I'd prefer a slice.

I agree that the underlying type should be a slice instead of a fixed sized array (which again assumes we use a specific key format/size).
I think we'll need to make this transition anyways if we want to define an interface for signing/VRF/hashing as proposed in #50 (or some related discussion I can't find right now).

@vqhuy
Copy link
Copy Markdown
Member Author

vqhuy commented Aug 12, 2016

If we don't, we assume the key to be of a specific format (like the one created by ed25519). Not returning the public-key here could make #50 more difficult to implement.

If we do, key.Public() seems to be redundant when I looked at it in the first time. Look at the code, the only use of key.Public() right now is for serialization function. Should key.Public() return a slice and be renamed to something like key.PublicPart()?

@liamsi
Copy link
Copy Markdown
Member

liamsi commented Aug 12, 2016

Look at the code, the only use of key.Public() right now is for serialization function. Should key.Public() return a slice and be renamed to something like key.PublicPart()?

I see your point. Then why not call it Serialize() like in other place we want a slice of byte from an object (used for serialization)? PublicPart sound like the public key is always part of the (encoding of the) private key. Are you referring to both Public (in vrf and sign) methods BTW?

@liamsi
Copy link
Copy Markdown
Member

liamsi commented Aug 12, 2016

They both use ed25519 but they have difference GenerateKey's signatures, and difference key type (sign's key type is a slice and vrf's key type is an array)

As a side-note: It would be fairly easy to modify the VRF lib to use sha512 instead of sha3/shake. Then we could use the exact same key type as in sign.go (type PrivateKey ed25519.PrivateKey) and GenerateKey could also look the same (basically calls ed25519.GenerateKey).

@arlolra
Copy link
Copy Markdown
Member

arlolra commented Aug 12, 2016

I suggest we should refactor the vrf package to export the same API's signature & key type as sign package. What do you think?

I think this is a fine idea. Can you open a separate ticket for it? I'd like to merge this code and I don't want the discussion to get lost.

If we don't, we assume the key to be of a specific format (like the one created by ed25519). Not returning the public-key here could make #50 more difficult to implement.

Not sure I agree. If, for whatever reason, the public key cannot be generated from the private key at call time, (private.Public()), we can always augment the PrivateKey structure to store it at construction.

@arlolra arlolra merged commit 80f787b into master Aug 12, 2016
@arlolra arlolra deleted the policy branch August 12, 2016 15:02
@arlolra
Copy link
Copy Markdown
Member

arlolra commented Aug 12, 2016

Can you open a separate ticket for it?

Sorry, I opened #65

@vqhuy
Copy link
Copy Markdown
Member Author

vqhuy commented Aug 12, 2016

Sorry, I opened #65

Thanks!

@arlolra arlolra restored the policy branch August 12, 2016 15:31
@arlolra arlolra deleted the policy branch August 12, 2016 15:32
@arlolra arlolra restored the policy branch August 12, 2016 15:32
@vqhuy vqhuy deleted the policy branch August 13, 2016 07:58
@vqhuy vqhuy restored the policy branch August 13, 2016 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants